Add telemetry for exporting variant analysis results#2186
Add telemetry for exporting variant analysis results#2186robertbrignull merged 4 commits intomainfrom
Conversation
koesie10
left a comment
There was a problem hiding this comment.
Could we instead move the withProgress call to be inside the exportVariantAnalysisResults function? Then we'd still only need to call this function, and don't need to inline any of the withProgress calls. I'd say that it's easier to understand if all commands are only called from 1 place (or none at all), rather than there being 1 or a few commands that are called from separate places.
|
Current problems/thoughts I'm having with this PR:
|
|
I've pushed some code but I'm not 100% sure of it. There's other stuff that needs doing but I'll come back later and have another go at this. |
koesie10
left a comment
There was a problem hiding this comment.
I think the code as-is looks good and correct. Like you mentioned there are some things that we can clean up, but we can also do this in follow-up PRs.
|
Yeah, let's just go for it. We can do more cleanup is followups. The remaining things I wanted to look at are:
|

This came about from wanting to deduplicate the usage of the
codeQL.exportVariantAnalysisResultscommand, however I came to the conclusion we should keep the use of this command and instead add extra telemetry to differentiate different usages of it.The reason I'm not proposing following the approach used in #2179 is because the
codeQL.exportVariantAnalysisResultshas a progress meter. This means it's not trivial to inline its implementation, because we'd have to callwithProgressand this seems non-ideal as that method is only meant to be used from a command.We call this command from three places:
codeQLQueryHistory.exportResultscommand, so this case already has telemetrycodeQL.exportSelectedVariantAnalysisResultscommand, so this case already has telemetryexportResultswebview messageSo by adding telemetry to the variant analysis results view we now have telemetry every place that triggers the
codeQL.exportVariantAnalysisResultscommand. This means the telemetry on the command itself is not needed to differentiate user actions and we can accept that it's used from multiple places.Does this reasoning make sense? Would it be better to instead use the approach from #2179 but work around it being a command with progress?
If this PR is merged, I can dismiss the code scanning alert for
codeQL.exportVariantAnalysisResultsand link to this PR in the dismissal comment.Checklist
ready-for-doc-reviewlabel there.